[FEATURE ember-application-engines] Initial extraction of engines from applications #12685

dgeb · 2015-12-05T16:29:59Z

This initial PR for engines follows the strategy proposed in the Engines RFC.

It introduces a base class, Engine, which is wholly extracted from (and then extended by) Application. Similarly, EngineInstance has been wholly extracted from (and then extended by) ApplicationInstance.

The Engine base class is now responsible for maintaining a registry and initializers.

The EngineInstance base class is now responsible for maintaining an instance-level registry and container.

Ember.Engine is only exposed when the new ember-application-engines feature flag is enabled.

The APIs and behavior of Application and ApplicationInstance remain identical, as do tests and documentation.

Some tests of extracted behavior were duplicated for engines, but the original tests for applications were kept in place. Any test duplication should be revisited before the feature flag is enabled.

The primary purpose of this PR is to allow for further experimentation with engines in an addon. It will also allow behavior to be folded into ember core gradually as experimentation proceeds.

mixonic · 2015-12-06T16:49:28Z

packages/ember-application/lib/system/application-instance.js

@@ -83,24 +79,20 @@ let ApplicationInstance = EmberObject.extend(RegistryProxy, ContainerProxy, {
  rootElement: null,

  init() {
-    this._super(...arguments);
+    let application = get(this, 'application');


Interacting with this before super() in a constructor is not compatible with ES6 classes, fwiw. If this can be differently factored, I think that would be ideal.

Good catch, @mixonic. I was able to refactor it such that the engine instance is aware that application might be set instead of base (i.e. if base is not set, then application is checked). if others agree that base is general enough to specify the base class for both engine and application instances, we can consider deprecating the application property at some point.

kiwiupover · 2015-12-06T18:36:17Z

Thanks for your work on this @dgeb Cheers

stefanpenner · 2015-12-06T20:16:07Z

packages/ember-application/lib/system/engine-instance.js

+  @uses ContainerProxyMixin
+*/
+
+let EngineInstance = EmberObject.extend(RegistryProxy, ContainerProxy, {


dgeb · 2015-12-06T21:00:40Z

@stefanpenner thanks for the review. I've just revised as per your suggestions, with the two exceptions:

Need to refactor registry.set = set;. Probably best in a separate PR since it's a change to current behavior.
Need to consider whether to deprecate usage of the application property in favor of base. For now, the EngineInstance accepts either as valid, so ApplicationInstance does as well.

@rwjblue thoughts?

rwjblue · 2015-12-08T03:02:43Z

packages/ember-application/lib/index.js

+  Ember.Engine = Engine;
+
+  // Expose EngineInstance for easy overriding. Reanalyze whether to
+  // continue exposing it after feature flag is removed.


I agree, we should likely remove it. ApplicationInstance is not exposed at all, so we should follow that (or expose it as well).

I debated exposing ApplicationInstance here as well - maybe we should for consistency and then remove both after the feature flag is removed?

I don't care which way we go, but we need to be consistent. Either both are exposed, or neither are...

rwjblue · 2015-12-08T03:06:02Z

The doc strings for Ember.Application need to be updated to properly mark that it @extends Ember.Engine (other wise the docs moved simply "vanish"). Same for Ember.EngineInstance.

rwjblue · 2015-12-08T03:31:24Z

packages/ember-application/lib/system/engine.js

+  return properties;
+}
+
+/**


Can you add a small description here? This is the header page that shows up for the class in our API docs...

dgeb · 2015-12-08T03:33:48Z

The doc strings for Ember.Application need to be updated to properly mark that it @extends Ember.Engine (other wise the docs moved simply "vanish"). Same for Ember.EngineInstance.

@rwjblue Good catch. I just updated @extends for Application. I think ApplicationInstance and EngineInstance are correct though (but let me know).

rwjblue · 2015-12-08T03:40:11Z

I think ApplicationInstance and EngineInstance are correct though (but let me know).

Confirm.

…neInstance

…n to Engine. Initializers and instance initializers are a common concern for applications and engines.

…ncerns for engines. Provide a spare base implementation of `buildRegistry` for `Engine`, which is extended by `Application`. Also, allow for a custom `Resolver` property on `Engine` (and by extension, `Application`).

…tialization for engine instances. Move registry and container creation from `ApplicationInstance` to `EngineInstance`.

Only expose `Ember.Engine` when feature flag is enabled.

Replicate appropriate tests from applications. We don’t want to yet move tests while engines are feature flagged and in flux.

Replicate appropriate tests from application instances. We don’t want to yet move tests while engines are feature flagged and in flux.

…ations.

…mber.ApplicationInstance. Expose Ember.EngineInstance and Ember.ApplicationInstance for easy overriding. Need to reanalyze whether to continue exposing them after feature flag is removed.

dgeb · 2015-12-08T04:24:09Z

@rwjblue I've added a brief description for Engine and exposed Ember.ApplicationInstance alongside Ember.EngineInstance (just while feature flagged).

Thanks for the review - let me know if I've overlooked any other suggestions.

[FEATURE ember-application-engines] Initial extraction of engines from applications

mixonic reviewed Dec 6, 2015
View reviewed changes

dgeb force-pushed the engines branch from 803341e to 840c42c Compare December 6, 2015 17:15

stefanpenner reviewed Dec 6, 2015
View reviewed changes

dgeb force-pushed the engines branch from 16a63e0 to d804f22 Compare December 6, 2015 22:54

rwjblue reviewed Dec 8, 2015
View reviewed changes

dgeb force-pushed the engines branch from d804f22 to 9520d22 Compare December 8, 2015 03:31

rwjblue reviewed Dec 8, 2015
View reviewed changes

dgeb added 10 commits December 7, 2015 22:19

[FEATURE ember-application-engines] Initial addition of Engine + Engi…

ffa7ab1

…neInstance

[FEATURE ember-application-engines] Move initializers from Applicatio…

6efba8b

…n to Engine. Initializers and instance initializers are a common concern for applications and engines.

[FEATURE ember-application-engines] Refactor container / registry ini…

9f0c63a

…tialization for engine instances. Move registry and container creation from `ApplicationInstance` to `EngineInstance`.

[FEATURE ember-application-engines] Feature flag for Ember.Engine.

21e86e0

Only expose `Ember.Engine` when feature flag is enabled.

[FEATURE ember-application-engines] Tests for engines.

2a4ead9

Replicate appropriate tests from applications. We don’t want to yet move tests while engines are feature flagged and in flux.

[FEATURE ember-application-engines] Tests for engine instances.

c6dc278

Replicate appropriate tests from application instances. We don’t want to yet move tests while engines are feature flagged and in flux.

[FEATURE ember-application-engines] Use const for module-level declar…

c17979d

…ations.

[FEATURE ember-application-engines] Remove unnecessary get/set calls.

cb4a674

[FEATURE ember-application-engines] Expose Ember.EngineInstance and E…

18b0a61

…mber.ApplicationInstance. Expose Ember.EngineInstance and Ember.ApplicationInstance for easy overriding. Need to reanalyze whether to continue exposing them after feature flag is removed.

dgeb force-pushed the engines branch from 9520d22 to 18b0a61 Compare December 8, 2015 04:21

rwjblue added a commit that referenced this pull request Dec 8, 2015

Merge pull request #12685 from dgeb/engines

192fceb

[FEATURE ember-application-engines] Initial extraction of engines from applications

rwjblue merged commit 192fceb into emberjs:master Dec 8, 2015

dgeb deleted the engines branch December 8, 2015 04:50

This was referenced Dec 8, 2015

Introduce select test helper #12450

Closed

Copy edit comment text of application.js #12522

Closed

thomascchen mentioned this pull request Dec 8, 2015

Ember engines - what are they? greenfieldhq/book-club#9

Open

dgeb mentioned this pull request Dec 21, 2015

RFC: Engines emberjs/rfcs#10

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FEATURE ember-application-engines] Initial extraction of engines from applications #12685

[FEATURE ember-application-engines] Initial extraction of engines from applications #12685

dgeb commented Dec 5, 2015

mixonic Dec 6, 2015

dgeb Dec 6, 2015

kiwiupover commented Dec 6, 2015

stefanpenner Dec 6, 2015

dgeb commented Dec 6, 2015

rwjblue Dec 8, 2015

dgeb Dec 8, 2015

rwjblue Dec 8, 2015

rwjblue commented Dec 8, 2015

rwjblue Dec 8, 2015

dgeb commented Dec 8, 2015

rwjblue commented Dec 8, 2015

dgeb commented Dec 8, 2015

[FEATURE ember-application-engines] Initial extraction of engines from applications #12685

[FEATURE ember-application-engines] Initial extraction of engines from applications #12685

Conversation

dgeb commented Dec 5, 2015

mixonic Dec 6, 2015

Choose a reason for hiding this comment

dgeb Dec 6, 2015

Choose a reason for hiding this comment

kiwiupover commented Dec 6, 2015

stefanpenner Dec 6, 2015

Choose a reason for hiding this comment

dgeb commented Dec 6, 2015

rwjblue Dec 8, 2015

Choose a reason for hiding this comment

dgeb Dec 8, 2015

Choose a reason for hiding this comment

rwjblue Dec 8, 2015

Choose a reason for hiding this comment

rwjblue commented Dec 8, 2015

rwjblue Dec 8, 2015

Choose a reason for hiding this comment

dgeb commented Dec 8, 2015

rwjblue commented Dec 8, 2015

dgeb commented Dec 8, 2015